Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename endpoints to use hyphens not underscores #1891

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Rename endpoints to use hyphens not underscores #1891

wants to merge 4 commits into from
+28 −24

Conversation

tawandamoyo
Copy link
Collaborator

@tawandamoyo tawandamoyo commented Oct 18, 2024 •
edited
Loading

Fixes #1863

requires

  • frontend update of edited endpoints - pr opened at #2123

@tawandamoyo tawandamoyo marked this pull request as draft October 18, 2024 16:04
@anth-volk
Copy link
Collaborator

Just quickly, could you change "Targeting" to "Fixes," either now or when you convert out of draft? "Targeting" actually doesn't close any issues.

@anth-volk
Copy link
Collaborator

And just to check: were you also willing to open a PR with the required changes on the front end?

@tawandamoyo tawandamoyo changed the title Fix/1863 rename endpoints to use hyphens not underscores Rename endpoints to use hyphens not underscores Oct 22, 2024
@tawandamoyo
Copy link
Collaborator Author

tawandamoyo commented Oct 22, 2024 •
edited
Loading

And just to check: were you also willing to open a PR with the required changes on the front end?

Yes. Opened #2122

@tawandamoyo tawandamoyo marked this pull request as ready for review October 22, 2024 18:15
@anth-volk
Copy link
Collaborator

Considering the significance of these changes, I'm going to wait to review/merge until very late Monday for a couple reasons:

  • Two of these require a bit of reconfiguration on Google Cloud to ensure we do liveness checks properly
  • There's always a chance that we miss some front-end fetch change, thereby temporarily breaking things

@tawandamoyo tawandamoyo mentioned this pull request Oct 29, 2024
Open
anth-volk
anth-volk requested changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @tawandamoyo. Before pushing, I wanted to check to see how this would impact our Google Cloud deployment, and I think this would crash our health checks. Following the documentation here, could you update gcp/policyengine_api/app.yaml to utilize the new readiness and liveness check URLs? I think you'd only need to update the path value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anth-volk anth-volk anth-volk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename endpoints containing underscores to use hyphens
2 participants
@tawandamoyo @anth-volk